Skip to content

fix(valkey): seed on the current master, not a reachable replica#687

Merged
mberlofa merged 2 commits into
helmforgedev:mainfrom
mreho:fix/valkey/masterDiscovery
Jul 6, 2026
Merged

fix(valkey): seed on the current master, not a reachable replica#687
mberlofa merged 2 commits into
helmforgedev:mainfrom
mreho:fix/valkey/masterDiscovery

Conversation

@mreho

@mreho mreho commented Jul 4, 2026 •
edited by mberlofa
Loading

Copy link
Copy Markdown
Contributor

Summary

Problem

The Sentinel bootstrap seeds on the node currently reporting role:master, but the wait loop only ran that discovery when the static seed (node-0) was unreachable. After a failover, node-0 can be a live replica, so a reachability-only check can seed Sentinel from a read-only node and advertise the wrong master.

Fix

Discover the node reporting role:master among the data-node peers first, and fall back to the static seed only if none answers within the bounded budget.

Type Of Change

  • New chart
  • Existing chart change
  • Documentation only
  • CI / repository workflow

PR Governance

  • I linked an existing issue in this PR body (Resolves #NNN or Related to #NNN)
  • If this is a new chart PR, labels enhancement and type:feature are applied

Checklist

  • I created this branch from updated main
  • My PR targets main
  • My commit message and PR title follow Conventional Commits
  • I did not edit version in Chart.yaml manually
  • I updated chart docs for behavior or default changes

Site Sync (GR-007)

Local Validation

  • make validate-chart CHART=valkey TIMEOUT=1200 passed end-to-end: FULLY VALIDATED (21 layers), including k3d behavioral scenarios for default, cluster, dual-stack, existing-secret, external-secrets, metrics, networkpolicy, replication, sentinel, standalone, tls-cluster, and tls-replication.
  • make site-sync-check CHART=valkey passed on site#365.
  • make release-check REPO=charts passed with the expected GR-077 post-merge release confirmation warning.
  • make attribution-check REPO=charts passed.

Remote Validation

  • GitHub Actions are green; standards-check is skipped by fork policy and covered locally.
  • CodeRabbit completed with no actionable comments.
  • Review threads: 0 unresolved.

Issue: #633

@coderabbitai

coderabbitai Bot commented Jul 4, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The Sentinel entrypoint's initial bootstrap logic in the Valkey Helm chart's configmap is modified to discover the current master by repeatedly calling find_current_master instead of pinging the seed node, falling back to SEED_MASTER after bounded attempts. Corresponding Helm test assertions are updated to match the new script content.

Changes

Sentinel Bootstrap Discovery Update

Layer / File(s) Summary
Discovery loop rewrite
charts/valkey/templates/configmap.yaml		 Replaces ping-based seed reachability loop with repeated find_current_master calls; falls back to SEED_MASTER and updates logging after 6 unsuccessful iterations.
Test assertions updated
charts/valkey/tests/cluster_domain_test.yaml, charts/valkey/tests/ha_extension_test.yaml		 Adds/updates matchRegex assertions to validate the new find_current_master-based master assignment logic in the entrypoint script.

Estimated code review effort: 2 (Simple) | ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: seeding from the current master instead of a reachable replica.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@mberlofa mberlofa mentioned this pull request Jul 4, 2026
Merged
@mberlofa mberlofa merged commit 97c6f50 into helmforgedev:main Jul 6, 2026
18 checks passed
mberlofa added a commit to helmforgedev/site that referenced this pull request Jul 6, 2026
@mberlofa
docs(valkey): sync failover hardening options (#365)
e7b2228 
## Summary

- document Valkey Sentinel graceful failover and startup failover guard
defaults
- add Valkey to the playground allowlist
- expose Sentinel pod policy and failover guard controls in the
playground

## Validation

- `make site-sync-check CHART=valkey`
- `npm run lint`
- `npm run format:check`
- `npm run build`
- `make release-check REPO=site`
- `make attribution-check REPO=site`

Chart PRs: helmforgedev/charts#664, helmforgedev/charts#687
Issue: helmforgedev/charts#633

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **New Features**
  * Added the Valkey chart to the playground chart selector.
* Expanded Valkey Sentinel settings with controls for
`podManagementPolicy`, `gracefulFailover` (enable + max attempts), and
`startupFailoverGuard` (enable + max attempts).

* **Documentation**
* Updated Valkey chart docs with Sentinel failover hardening guidance,
including how graceful failover is bounded and how startup failover
protection prevents unsafe resumption.
* Added the new Sentinel settings to the values reference table,
including upgrade-compatibility notes for `podManagementPolicy`.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

area:templates area:tests chart:valkey size:S

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@mreho @mberlofa